-
Notifications
You must be signed in to change notification settings - Fork 94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(replays): add replay_id onto event from dynamic sampling context #1983
Conversation
need to look into the unit test failures, and address the replay context object usage in the replay event payload. |
Co-authored-by: Oleksandr <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general this looks good to me. I would just move the logic to store normalization.
relay-server/src/actors/processor.rs
Outdated
if let Some(dsc) = envelope.dsc() { | ||
if let Some(replay_id) = dsc.replay_id { | ||
let contexts = event.contexts.get_or_insert_with(Contexts::new); | ||
contexts.add(SentryContext::Replay(Box::new(ReplayContext { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This overwrites a replay context if it was already on the event. Is that fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup, the client should not be setting the replay context on events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The move to normalization looks good, but that means that enrich_event_with_dsc
can now be removed, right? After that, this should be good to merge.
Please also remove the pending snapshot file, and run cargo insta review
to fix the test failures.
okay should be good! thanks for the review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ReplayContext
that will contain the replay_id on events (errors and transactions, etc.). Note that the SDKs never add this value to the event, it is always added in relay via the DSC.Related: getsentry/rfcs#60 getsentry/team-webplatform-meta#41
getsentry/sentry#45529
Closes: #1958